feat: Add Pagination for server requests - #140
Conversation
4c05e1a to
8d48b93
Compare
chemicL
left a comment
There was a problem hiding this comment.
Thank you for the contribution! It is a great start, would you like to take this further and unify the logic for other listing requests?
|
Hi, thanks for your review and your feedback!
Yes I will do this. I should be able to update the pull request by the end of the week. |
3dce44d to
0c93c71
Compare
0c93c71 to
b02db82
Compare
|
I did another push with small changes in the commit title and message and have also changed a few comments and names in tests. No business logic was affected. |
3718965 to
5f7bb9b
Compare
Adds the Pagination feature as described in the specification for prompts, resources, resource templates and tools. To make this possible mainly two changes are made: 1. The logic for cursor handling is added. 2. Handling for invalid parameters (MCP error code `-32602 (Invalid params)`) is added to the `McpServerSession`. For now the cursor is the base64 encoded start index of the next page and the hash value of the collection at time of computing. The page size is set to 10. When parameters are found to be invalid the newly introduced `McpParamsValidationError` is returned to handle it properly in the `McpServerSession`.
5f7bb9b to
094088d
Compare
|
@denniskawurek Is this change still being worked on? I am making some changes to handle pagination on the client side and just saw that the server side changes are yet to be merged. I can pick up the remaining work on this PR if you need some help. |
|
I've just merged main into the branch, so that it's up to date, again. Regarding your question I haven't started anything in regards of the client, yet. Maybe @chemicL can give some information of the maintainer point of view and whether this change is interesting. Regards |
|
Looks like the test is flaky somehow. However, I'm not allowed to rerun the job. |
|
Hi, thanks for putting this work together. I am very interested in seeing PR merged as it's blocking some of my work right now. What needs to be done to move this along? Thanks in advance! |
|
Resolves #967 |
Kehrlann
left a comment
There was a problem hiding this comment.
Hey @denniskawurek , picking this up from #967
A few concerns here:
- It's a breaking change ; we should default to non-paginated results
- Page size should be configurable
- We've introduced tool filtering as part of #1108 ; so now the view is request dependent. If that PR gets merged first, you'll need to adapt the pagination to happen after the filtering (shouldn't be too hard)
| var cursor = paginatedRequest != null ? paginatedRequest.cursor() : null; | ||
|
|
||
| var mapSize = this.tools.size(); | ||
| var mapHash = this.tools.hashCode(); |
There was a problem hiding this comment.
This is likely wrong across horizontally-scaled instances - hashCode hashes the *ToolSpecification, which contain handlers, and the hash of those will be tied to object identity.
Additionally, that's hashing a lot of things, including input and output schema strings, without ever caching them.
We should probably hash across keys (tool names), but I guess we have no guarantee of tool names being unique so there's something to do about this too
| var resultList = this.resources.values() | ||
| .stream() |
There was a problem hiding this comment.
I think the ordering is not guaranteed to be stable in a ConcurrentHashMap. I think in practice it's stable, BUT if you start adding elements dynamically and grow the underlying map, you may end up with different orders. I don't think it's a massive problem in this specific case.
If we do order by key as suggested in tools, we can sort the keys and get a stability guarantee (unless you modify the collection during a pagination run, but that's a corner case.)
| private BiFunction<McpSyncServerExchange, McpSchema.CallToolRequest, CallToolResult> buildCallToolRequestHandlerMock() { | ||
| var callResponse = McpSchema.CallToolResult.builder() | ||
| .addContent(McpSchema.TextContent.builder("CALL RESPONSE").build()) | ||
| .build(); | ||
|
|
||
| return (exchange, request) -> { | ||
| // perform a blocking call to a remote service | ||
| try { | ||
| HttpResponse<String> response = HttpClient.newHttpClient() | ||
| .send(HttpRequest.newBuilder() | ||
| .uri(URI.create( | ||
| "https://raw.githubusercontent.com/modelcontextprotocol/java-sdk/refs/heads/main/README.md")) | ||
| .GET() | ||
| .build(), HttpResponse.BodyHandlers.ofString()); | ||
| String responseBody = response.body(); | ||
| assertThat(responseBody).isNotBlank(); | ||
| } | ||
| catch (Exception e) { | ||
| e.printStackTrace(); | ||
| } | ||
| return callResponse; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Is there a reason for reaching out to a remote service in tests? I'd rather avoid that and just return a result.
Adds the Pagination feature to the
prompts/listfeature to fulfil the specification.Motivation and Context
This adds the Pagination feature to the
prompts/listfeature as described in the specification.To make this possible mainly two changes are made:
-32602 (Invalid params)) is added to theMcpServerSession.Decisions made:
base64string.10.McpParamsValidationErroris returned to handle it properly in theMcpServerSession.If this change is ok, I'm happy to create a follow-up PR for the other MCP features. Maybe some parts of the cursor handling can be moved out to separate methods.
How Has This Been Tested?
Integration tests are added for the
prompts/listfeature.Types of changes
Checklist
Resolves #967